Skip to content

JMXFetch bundled integrations disabled by default - #696

Merged
tylerbenson merged 1 commit into
masterfrom
tyler/jmxfetch-config
Feb 7, 2019
Merged

JMXFetch bundled integrations disabled by default#696
tylerbenson merged 1 commit into
masterfrom
tyler/jmxfetch-config

Conversation

@tylerbenson

Copy link
Copy Markdown
Contributor

Enable each individually by setting -Ddd.integration.<integration_name>.enabled=true.

@tylerbenson tylerbenson added type: feature Enhancements and improvements inst: others All other instrumentations labels Feb 6, 2019
@tylerbenson tylerbenson added this to the 0.23.0 milestone Feb 6, 2019
@tylerbenson
tylerbenson requested a review from mar-kolya February 6, 2019 17:39
@tylerbenson
tylerbenson force-pushed the tyler/jmxfetch-config branch 2 times, most recently from 8ca167d to 9e65b7c Compare February 6, 2019 18:37

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the defaultEnabled have to be boxed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This allows null to be passed in for no default.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, that doesn't really work, so I'll change it to primitive.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you cast it to primitive anyway. And besides caller should make up their mind about this default :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks odd. Did you mean to test system property and env variable separately? Otherwise it is unclear if they actually work independently. Also you may want to check that false value here works as expected.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are tested separately by virtue of using different values. I did add more test cases though.

@mar-kolya mar-kolya left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of minor comments that should be addressed, otherwise LGTM

Enable each individually by setting `-Ddd.integration.<integration_name>.enabled=true`.
@tylerbenson
tylerbenson force-pushed the tyler/jmxfetch-config branch from 9e65b7c to 4414dc2 Compare February 6, 2019 21:01
@tylerbenson
tylerbenson merged commit 0802bb8 into master Feb 7, 2019
@tylerbenson
tylerbenson deleted the tyler/jmxfetch-config branch February 7, 2019 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

inst: others All other instrumentations type: feature Enhancements and improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants